Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: fix stat dev unsigned cast overflow #16705

Closed
wants to merge 2 commits into from

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Nov 3, 2017

The dev_t is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow

Fixes: #16496

Closes #16667 since this PR is a Semver-Patch alternatives regarding to #16496 only.

According to the linux headers, (http://elixir.free-electrons.com/linux/v4.13.11/source/include/linux/types.h#L15, http://elixir.free-electrons.com/linux/v4.13.11/source/include/uapi/asm-generic/posix_types.h#L23), both dev_t, mode_t, nlink_t are unsigned. Using Integer::NewFromUnsigned is more appropriate.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow

Fixes: nodejs#16496
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. v6.x labels Nov 3, 2017
@JLHwung JLHwung mentioned this pull request Nov 3, 2017
4 tasks
@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2017

FWIW all of the values should probably just use Number::New() since libuv has all of the uv_stat_t fields as uint64_t. Yes, double can only give us at most 53 bits, but that's more than 32. In fact, this is what the master branch (and v8.x) already does.

@JLHwung
Copy link
Contributor Author

JLHwung commented Nov 3, 2017

@mscdex Fixed, thank you.
As long as we won't come across to a huge fs with more than 53 bit dev ID before the ending of v6 LTS, I think it is sufficient for pragmatical usage.

@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2017

I'm not sure how other @nodejs/collaborators feel about the semver-ness of the following suggestion: I think maybe we should just use -1 for the blksize and blocks fields on non-POSIX platforms, which matches the FillStatsArray() behavior.

#define X(name) \
Local<Value> name = Integer::NewFromUnsigned(env->isolate(), s->st_##name); \
Local<Value> name = Number::New(env->isolate(), \
static_cast<double>(s->st_##name)); \
Copy link
Member

@bnoordhuis bnoordhuis Nov 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed the name.IsEmpty() check but it should most definitely stay.

(edit: looking at the wrong diff.)

// Unsigned integers. It does not actually seem to be specified whether
// uid and gid are unsigned or not, but in practice they are unsigned,
// and Node’s (F)Chown functions do check their arguments for unsignedness.
// Numbers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment looks pretty out of place now. Care to just remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are added on e9ce8fc to accompany different X macro definition. I don't think it is out of place and we are good to let it be.

@MylesBorins
Copy link
Contributor

MylesBorins pushed a commit that referenced this pull request Nov 26, 2017
The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow

PR-URL: #16705
Fixes: #16496
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2017
PR-URL: #16705
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 659c458...8ade20f

MylesBorins pushed a commit that referenced this pull request Nov 28, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow

PR-URL: #16705
Fixes: #16496
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
PR-URL: #16705
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 28, 2017
JLHwung added a commit to hexojs/hexo that referenced this pull request Dec 6, 2017
This reverts commit a218422.

Since nodejs/node#16705 is merged, the hotfix is unecessary.

# Conflicts:
#	test/scripts/box/file.js
@JLHwung JLHwung deleted the fix-16496 branch December 6, 2017 14:56
JLHwung added a commit to hexojs/hexo that referenced this pull request Dec 7, 2017
This reverts commit a218422.

Since nodejs/node#16705 is merged, the hotfix is unecessary.
JLHwung added a commit to hexojs/hexo that referenced this pull request Dec 8, 2017
This reverts commit a218422.

Since nodejs/node#16705 is merged, the hotfix is unecessary.
JLHwung added a commit to hexojs/hexo that referenced this pull request Dec 10, 2017
This reverts commit a218422.

Since nodejs/node#16705 is merged, the hotfix is unecessary.
NoahDragon pushed a commit to hexojs/hexo that referenced this pull request Dec 16, 2017
* Revert "hotfix(file): cast dev to uint32"

This reverts commit a218422.

Since nodejs/node#16705 is merged, the hotfix is unecessary.

* test(appveyor): update to exact node.js version
thom4parisot pushed a commit to thom4parisot/hexo that referenced this pull request Jan 17, 2020
* Revert "hotfix(file): cast dev to uint32"

This reverts commit a218422.

Since nodejs/node#16705 is merged, the hotfix is unecessary.

* test(appveyor): update to exact node.js version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants